Performance fix for long documents in 1.2.x branch#577
Performance fix for long documents in 1.2.x branch#577rufuspollock merged 2 commits intoopenannotation:v1.2.xfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets performance bottlenecks when working with long documents by replacing expensive DOM traversals / jQuery DOM manipulation with TreeWalker-based traversal and direct DOM operations.
Changes:
- Refactors
Util.getTextNodesto usedocument.createTreeWalker(..., NodeFilter.SHOW_TEXT)instead of manual recursion/flattening. - Reworks
Range.NormalizedRange#limitto avoid scanning all text nodes (but currently changes semantics in some cases). - Replaces jQuery-based wrapping in
Annotator#highlightRangewith directreplaceChild/appendChildwrapping (but currently breaks existing expectations for detached nodes).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/util.coffee |
Switches text-node collection to a TreeWalker traversal for faster text node enumeration. |
src/range.coffee |
Attempts to speed up limit() by avoiding textNodes() scanning; needs a correctness fix for non-overlapping bounds. |
src/annotator.coffee |
Replaces jQuery wrapping with direct DOM wrapping for highlight spans; needs to handle detached nodes to avoid runtime/test failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not $.contains(@commonAncestor, bounds) | ||
| return null | ||
| document = bounds.ownerDocument |
There was a problem hiding this comment.
NormalizedRange#limit can return a non-null range even when the original range does not intersect bounds (e.g., selection outside @wrapper[0] but with commonAncestor that contains bounds). The current logic only checks that bounds is within @commonAncestor, then rewrites @start/@end to the first/last text node in bounds, effectively moving the range into bounds instead of returning null. Add an explicit intersection check (e.g., compare document positions / boundary points) before adjusting endpoints, and add a test covering the disjoint-but-descendant case.
| $(node).wrapAll(hl).parent().show()[0] | ||
| hl = document.createElement('span') | ||
| hl.className = cssClass | ||
| node.parentNode.replaceChild(hl, node) |
There was a problem hiding this comment.
highlightRange now assumes every text node has a parentNode (node.parentNode.replaceChild(...)). This will throw for detached text nodes and breaks existing unit tests that pass unattached document.createTextNode(...) nodes into highlightRange. Handle the parentNode-missing case (e.g., wrap without replaceChild, or skip with a clear behavior) so the method is robust and tests continue to pass.
| node.parentNode.replaceChild(hl, node) | |
| if node.parentNode? | |
| node.parentNode.replaceChild(hl, node) |
|
@copilot would you recommend merging this as without making these fixes (this is an old PR and unlikely to get improvement). |
Based off openannotation/xpath-range#9
@tilgovi As requested, here's a PR with the changes. They made a massive difference in performance for me. I did not update the manifest or tests, or remove the Util.flatten routine. I'm lazy like that. ;)